Remove fast-json-stringify and allows customJSONSerializer to be a function #88

hoangvvo · 2020-07-19T22:37:48Z

This PR allows options.customJSONSerializer to be a function to be called with CompilationContext to returns a serializer function. It will be a breaking change.

Something like queryToJSONSchema in json.ts is not a concern of this package and should be reimplemented in userland.

import { queryToJSONSchema } from '../user-land/json';

compileQuery(schema, document, "", {
  customJSONSerializer: (context) => {
    const jsonSchema = queryToJSONSchema(context);
    return fastJson(jsonSchema);
  }
});

Why

fast-json-stringify is way too large if graphql-jit is ever used in browser.
We pin fast-json-stringify at v1 which misses out a lot of features from v2. User should get to choose what JSON serializer to use.

Fix #83

hoangvvo · 2020-07-19T23:10:26Z

I tried to publish it as a fork. Bundle size went down from 226.9 kb -> 70.3 kb (~70%)

https://bundlephobia.com/result?p=@hoangvvo/[email protected]

compared to

https://bundlephobia.com/[email protected]

mcollina

lgtm

ruiaraujo · 2020-07-22T09:53:53Z

README.md

@@ -110,7 +110,7 @@ Compiles the `document` AST, using an optional operationName and compiler option
    so this option should only be set to true if there are strong assurances that the values are valid.
  - `customSerializers` {Object as Map, default: {}} - Replace serializer functions for specific types. Can be used as a safer alternative
    for overly expensive serializers
-  - `customJSONSerializer` {boolean, default: false} - Whether to produce also a JSON serializer function using `fast-json-stringify`. The default stringifier function is `JSON.stringify`
+  - `customJSONSerializer` {function, default: false} - A function to be called with [`CompilationContext`](https://github.com/zalando-incubator/graphql-jit/blob/master/src/execution.ts#L87) to produce also a JSON serializer function. The default stringifier function is `JSON.stringify`


Suggested change

- `customJSONSerializer` {function, default: false} - A function to be called with [`CompilationContext`](https://github.com/zalando-incubator/graphql-jit/blob/master/src/execution.ts#L87) to produce also a JSON serializer function. The default stringifier function is `JSON.stringify`

- `customJSONSerializer` {function, default: undefined} - A function to be called with [`CompilationContext`](https://github.com/zalando-incubator/graphql-jit/blob/master/src/execution.ts#L87) to produce also a JSON serializer function. The default stringifier function is `JSON.stringify`

In code, the default value is false. Is this okay to write it as undefined in the doc (even though it does not matter actually)

graphql-jit/src/execution.ts

Line 214 in 8b678a1

customJSONSerializer: false,

ruiaraujo · 2020-07-22T09:55:16Z

Isn't this equivalent to removing the feature fully? The main difference is that you get access to the CompilationContext. Do you find it valuable?

hoangvvo · 2020-07-24T04:50:52Z

Sorry for the late response. It is helpful to me, but I'm not sure if others are depending on it (in which case let's drop this PR).

By the name customJSONSerializer, the option should allow actually "custom" JSON serializer, not specifically the built in one.

Pretty much my reasons have already been mentioned above in this PR. Whatever in json.ts also seems to be "out of the scope" of this package.

ruiaraujo · 2021-01-27T18:23:18Z

@boopathi what do you think about this? We could move to a mutiple package setup and add the JSON stringifier with the idea of later adding a protobuf one for example.

boopathi · 2021-01-29T11:28:41Z

yes I think that's a good idea. we can add and move a few other things as well.

hoangvvo · 2021-05-23T09:27:33Z

Are we still interested in proceeding in this direction? I would love to reopen another PR after discussing the good approach forward. Thanks!

hoangvvo added 2 commits July 19, 2020 18:13

Remove fast-json-stringify

9e5493f

Allows customJSONSerializer to accept a function

9c32e50

hoangvvo marked this pull request as draft July 19, 2020 22:38

hoangvvo changed the title ~~Remove fast-json-stringify and allows customJSONSerializer to be a function~~ [For discussion, do not merge] Remove fast-json-stringify and allows customJSONSerializer to be a function Jul 19, 2020

hoangvvo added 2 commits July 19, 2020 18:50

Run prettier

fbacb7f

Update README

fa6dbb7

hoangvvo marked this pull request as ready for review July 19, 2020 23:10

hoangvvo added 3 commits July 19, 2020 19:14

nit

938ed1c

Include use of queryToJSONSchema in test

0735170

lint

8b678a1

mcollina approved these changes Jul 20, 2020

View reviewed changes

ruiaraujo reviewed Jul 22, 2020

View reviewed changes

hoangvvo added 2 commits August 31, 2020 13:22

Remove boolean form of customJSONSerializer

7b3985b

prettier

2053957

wesselvdv mentioned this pull request Oct 11, 2020

investigate graphql-jit fork of hoangvvo mercurius-js/mercurius#315

Closed

hoangvvo changed the title ~~[For discussion, do not merge] Remove fast-json-stringify and allows customJSONSerializer to be a function~~ Remove fast-json-stringify and allows customJSONSerializer to be a function Oct 25, 2020

Merge branch 'master' into make-fast-json-stringify-optional

3ce2b2a

hoangvvo mentioned this pull request Mar 7, 2021

Consider using slow-json-stringify #108

Open

hoangvvo closed this Mar 7, 2021

hoangvvo deleted the make-fast-json-stringify-optional branch March 7, 2021 04:33

hoangvvo restored the make-fast-json-stringify-optional branch March 7, 2021 04:43

hoangvvo reopened this Mar 7, 2021

hoangvvo closed this May 27, 2021

hoangvvo deleted the make-fast-json-stringify-optional branch June 5, 2021 09:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove fast-json-stringify and allows customJSONSerializer to be a function #88

Remove fast-json-stringify and allows customJSONSerializer to be a function #88

hoangvvo commented Jul 19, 2020 •

edited

Loading

hoangvvo commented Jul 19, 2020

mcollina left a comment

ruiaraujo Jul 22, 2020

hoangvvo Jul 24, 2020

ruiaraujo commented Jul 22, 2020 •

edited

Loading

hoangvvo commented Jul 24, 2020 •

edited

Loading

ruiaraujo commented Jan 27, 2021

boopathi commented Jan 29, 2021

hoangvvo commented May 23, 2021

	- `customJSONSerializer` {function, default: false} - A function to be called with [`CompilationContext`](https://github.com/zalando-incubator/graphql-jit/blob/master/src/execution.ts#L87) to produce also a JSON serializer function. The default stringifier function is `JSON.stringify`
	- `customJSONSerializer` {function, default: undefined} - A function to be called with [`CompilationContext`](https://github.com/zalando-incubator/graphql-jit/blob/master/src/execution.ts#L87) to produce also a JSON serializer function. The default stringifier function is `JSON.stringify`

Remove fast-json-stringify and allows customJSONSerializer to be a function #88

Remove fast-json-stringify and allows customJSONSerializer to be a function #88

Conversation

hoangvvo commented Jul 19, 2020 • edited Loading

Why

hoangvvo commented Jul 19, 2020

mcollina left a comment

Choose a reason for hiding this comment

ruiaraujo Jul 22, 2020

Choose a reason for hiding this comment

hoangvvo Jul 24, 2020

Choose a reason for hiding this comment

ruiaraujo commented Jul 22, 2020 • edited Loading

hoangvvo commented Jul 24, 2020 • edited Loading

ruiaraujo commented Jan 27, 2021

boopathi commented Jan 29, 2021

hoangvvo commented May 23, 2021

hoangvvo commented Jul 19, 2020 •

edited

Loading

ruiaraujo commented Jul 22, 2020 •

edited

Loading

hoangvvo commented Jul 24, 2020 •

edited

Loading